Initial implementation of SVS Runtime package #208
Conversation
…allow non-LTO linking
…el/ScalableVectorSearch into rfsaliev/cpp-runtime-binding
bindings/cpp/include/vamana_index.h
Outdated
| }; | ||
|
|
||
| // TODO: | ||
| // 1. Should StorageKind, metric, be a part of BuildParams? |
There was a problem hiding this comment.
StorageKind and metric should not be part of BuildParams. We use DataType and QueryType in SVS for storage kinds. If it's possible we can stick to those.
There was a problem hiding this comment.
Made StorageKind globally defined by moving svs::runtime namespace in IndexSVSImplDefs.h.
bindings/cpp/include/vamana_index.h
Outdated
| }; | ||
|
|
||
| struct SearchParams { | ||
| size_t search_window_size = 0; |
There was a problem hiding this comment.
We should add the two prefetching parameters here. Even if those will be kept to default values in general, it'd be good to have them here.
I think this produces more readable code. And since `runtime_error_wrapper()` is an internal function, there shouldn't be any disadvantages for the shared lib.
rfsaliev
requested review from
Alexsandruss,
ahuber21,
dian-lun-lin,
homksei,
ibhati and
yuejiaointel
as code owners
ahuber21
left a comment
ahuber21
left a comment
There was a problem hiding this comment.
Some minor comments. Let's discuss how to tackle todos.
| /// | ||
| /// @brief Version information and API versioning for SVS Runtime | ||
| /// | ||
| /// This header defines the SVS Runtime API versioning scheme similar to oneDAL: |
There was a problem hiding this comment.
The reference to oneDAL should be removed
| * limitations under the License. | ||
| */ | ||
|
|
||
| #include <svs/runtime/dynamic_vamana_index.h> |
There was a problem hiding this comment.
I suggest to sort all headers in the order
- "Local" headers (includes with quotes, same directory)
- SVS runtime headers
- SVS core headers
- deps headers
- std library, other core headers
Also, in svs/include we use mainly quote includes. IMO we should stay consistent here. Or are there reasons against?
| IDFilter* filter = nullptr | ||
| ) const noexcept override { | ||
| return runtime_error_wrapper([&] { | ||
| // TODO wrap arguments into proper data structures in DynamicVamanaIndexImpl and |
| IDFilter* filter = nullptr | ||
| ) const noexcept override { | ||
| return runtime_error_wrapper([&] { | ||
| // TODO wrap arguments into proper data structures in DynamicVamanaIndexImpl and |
| if (filter == nullptr) { | ||
| auto queries = svs::data::ConstSimpleDataView<float>(x, n, dim_); | ||
|
|
||
| // TODO: faiss use int64_t as label whereas SVS uses size_t? |
There was a problem hiding this comment.
Should be possible to remove this, because conversion is now handled at library boundary?
| for (; found < k; ++found) { | ||
| curr_distances[found] = -1; | ||
| curr_labels[found] = -1; |
There was a problem hiding this comment.
Is padding with -1 specific to Faiss? Would it make sense to accept a parameter to pick a different value for padding? Maybe even skip this entirely?
| Status search(size_t n, const float* x, size_t k, float* distances, size_t* labels) | ||
| const noexcept override { | ||
| return runtime_error_wrapper([&] { | ||
| // TODO wrap arguments into proper data structures in FlatIndexImpl and |
| if (filter == nullptr) { | ||
| auto queries = svs::data::ConstSimpleDataView<float>(x, n, dim_); | ||
|
|
||
| // TODO: faiss use int64_t as label whereas SVS uses size_t? |
There was a problem hiding this comment.
Resolve todo, same comment about type as above
| auto matrix = svs::leanvec::compute_leanvec_matrix<svs::Dynamic, svs::Dynamic>( | ||
| data, means, threadpool, svs::lib::MaybeStatic<svs::Dynamic>{leanvec_dims} | ||
| ); | ||
| // Create a copy of the matrix for the query matrix to avoid double free. |
There was a problem hiding this comment.
For my understanding: Explicit copy with 2x move gives 1 copy, passing twice by value 2 copies. So in interest of performance, we do it like this?
So, it should be equivalent to LeanVecMatricesType{std::move(matrix), matrix}; ?
| /bindings/python/dist/ | ||
|
|
||
| # CPP bindings build files | ||
| /bindings/cpp/build/ |
There was a problem hiding this comment.
| /bindings/cpp/build/ | |
| build | |
| build_* |
More generic?
| return make_storage<StorageType_t<Tag>>(std::forward<Args>(args)...); | ||
| } | ||
|
|
||
| inline StorageKind to_supported_storage_kind(StorageKind kind) { |
There was a problem hiding this comment.
Are we planning to make the behavior configurable? What if the user wants to throw an error instead. I don't know if a silent fallback is good behavior of the runtime...
No description provided.